Background

In our company's codebase, we make heavy use of the popular Immutables library to generate value objects. Our typical style looks like this:

@Immutable
@HubSpotStyle
public abstract class MyImmutableIF {
  public abstract int property();
}

From this definition, Immutables would generate a MyImmutable value class extending MyImmutableIF, and an associated builder. We tend to reference the generated type (MyImmutable in this case) in consuming code rather than the abstract definition type (MyImmutableIF). In hindsight, that usage pattern does complicate some things, but we now have thousands of these value classes so we'll likely need to support this pattern for the foreseeable future.

One downside of this usage pattern is it tends to lead to the chance of a class initialization deadlock. For example, often immutable creators want to have some default instance, and they'll define that on their abstract type:

@Immutable
@HubSpotStyle
public abstract class MyImmutableIF {
  public static final MyImmutable FORTY_TWO = MyImmutable.builder().setProperty(42).build();

  public abstract int property();
}

This is obviously prone to a class initialization deadlock (since MyImmutable extends MyImmutableIF, and MyImmutable FORTY_TWO is a static field of MyImmutableIF assigned to during the class's initialization), and we've been bitten by them many times in the past using this pattern. This was the inspiration for my fix in #4429, and with that in place ClassInitializationDeadlock correctly detects these situations.

One way we've worked around the possibility of deadlocks in these situations is to defer the construction of the default value past class initialization time:

@Immutable
@HubSpotStyle
public abstract class MyImmutableIF {
  private static final Supplier<MyImmutable> FORTY_TWO = Suppliers.memoize(
      () -> MyImmutable.builder().setProperty(42).build()
  );

  public static MyImmutable fortyTwo() {
    return FORTY_TWO.get();
  }

  public abstract int property();
}

(Using Guava's Suppliers.memoize)

This is not prone to class initialization deadlock since, according to the JVM spec and real-world experimentation, MyImmutable won't be initialized when MyImmutableIF is initialized. MyImmutable will only be initialized once MyImmutableIF.fortyTwo() is called since that's the first time something on MyImmutable is accessed (assuming it hasn't otherwise already been initialized before that, of course).

Another common example in our codebase is holding some common function object that operates on instances of the value class:

@Immutable
@HubSpotStyle
public abstract class MyImmutableIF {
  public static final Comparator<MyImmutable> COMPARATOR =
      Comparator.comparingInt(MyImmutable::property);

  public abstract int property();
}

This is also not prone to class initialization deadlock; referencing methods does not initialize the class containing the referenced method (at least not with javac; I think it's possible for that to differ depending on how method references are compiled).

However, ClassInitializationDeadlock (with my previous fix) flags both of these situations, the first at the usage of MyImmutable.builder() inside the lambda passed to Suppliers.memoize, and the second at the reference to MyImmutable::property.

The fix

This PR updates ClassInitializationDeadlock to ignore lambda expressions and member references when looking for references that could enable a class initialization deadlock.

I recognize this isn't a perfect fix. Some lambdas or member references can actually be problematic. Here's a contrived example:

class A {
  static Object cycle = ((Supplier) B::new).get();
}
class B extends A {}

With this PR, Error Prone wouldn't flag that usage despite it being prone to a class initialization deadlock.

However, I can't think of any useful usages of lambdas or member references that would both enable a class initialization deadlock and wouldn't otherwise reference the other class in a way that would initialize it anyway. I'm sure we can come up with some examples, but at least in our codebase, most of the method references and lambdas the check is flagging are false positives.

Thanks for your consideration.